[fix][broker] Return non-null persistence policy for namespace - #14158
[fix][broker] Return non-null persistence policy for namespace#14158rdhabalia wants to merge 4 commits into
Conversation
|
@rdhabalia:Thanks for your contribution. For this PR, do we need to update docs? |
1 similar comment
|
@rdhabalia:Thanks for your contribution. For this PR, do we need to update docs? |
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
Lgtm
With this change, is it possible to know from the API if we set a policy vs we are returning the default values?
|
this change only impacts api-get call code-path and it will not change set-policy with default values. |
Jason918
left a comment
Jason918
left a comment
There was a problem hiding this comment.
I think we can add an applied to control whether we should return namespace setting or the value that actually take effects, like many other polices in org.apache.pulsar.broker.admin.v2.PersistentTopics
Agree, with this change, users will not able to know what is the namespace settings. We should apply the same behavior, for policies, if the topic/namespace don't have a policy, just provide a response to tell users no policy for this topic/namespace, if users want to check which policy will be applied, they can add --applied`. |
|
I will move this one to milestone 2.11 first. |
this is incorrect, this PR only sets default persistence policy if the value is not configured and right now it's returning null which is confusing for user. we don't need any additional control flag to show null value to user. and regrading other policies , that can be handled in separate PR and we don't want to mix everything in this one. @codelipenghui this PR has minor change which doesn't break any change then why should we change the release for this PR and why can't we merge to 2.10 ? |
|
On a note completely unrelated to the content of this PR, we're able to merge this PR without a test passing because it is not "required". I am working on fixing this by updating the
|
|
The pr had no activity for 30 days, mark with Stale label. |
|
Reopened to trigger CI |
|
The pr had no activity for 30 days, mark with Stale label. |
|
The pr had no activity for 30 days, mark with Stale label. |
tisonkun
changed the title
github-actions
Bot
added
doc-not-needed
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14158 +/- ##
=============================================
- Coverage 46.34% 34.84% -11.51%
+ Complexity 10394 7480 -2914
=============================================
Files 703 703
Lines 68838 68840 +2
Branches 7379 7378 -1
=============================================
- Hits 31905 23988 -7917
- Misses 33324 41645 +8321
+ Partials 3609 3207 -402
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
It seems the codebase evolves a lot and the added test Close as stale. @rdhabalia you may pick up this patch and rebase on the current master to pass the related tests and resubmit it again. |
7 participants
Motivation
Right now, if namespace is not configured with persistence policy then broker considers default persistence-policy while serving the topic but it returns null value in get-persistence admin API
Modification
Return default persistence policy if namespace persistence policy is not configured.
doc-not-needed